Repository navigation
Conversation
… frame `ondatagramstatus(id, status)` gave a status to the wrong id as soon as a packet was declared lost ahead of acknowledged ones. lsquic reported `(count, acked)` for a packet, and the session popped that many ids from a send-order queue. `on_dg_write` now names the frame with `lsquic_conn_set_datagram_id`. `write_datagram` stores the id in the DATAGRAM frame record. The send controller reports each frame as `(id, status)` when its packet is acknowledged, when its packet is declared lost, and when lsquic drops the frame before the send. The session reports a frame that was dropped before the send as 'lost'. The session keeps no queue of in-flight ids and no `sent` event for each datagram.
|
Status
|
…n fails Each wait of the relay harness now races the `closed` promise and the `onerror` event of both sessions. A session that fails rejects the wait, and the test does not run into its timeout. The teardown destroys the server session, so a client that failed does not leave it to the idle timeout.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, two more things were checked and ruled out: a lost packet that lsquic reschedules does not report its datagram 'lost' a second time through the new PO_REPACKNO site (the DATAGRAM frame sits in po_regen_sz and is chopped by update_for_resending before the packet is scheduled again), and on_datagram_status writing to session.events directly skips nothing (push_event is a plain push, and the drain pops from the front, so coalescing on the last event cannot touch an event being dispatched). 7426614 wires every relay-harness wait to reject on session failure or close.
Extended reasoning...
The change reworks per-datagram status reporting across the lsquic patch (frame-record id, three report sites in lsquic_send_ctl.c), the C shim and Rust FFI vtable signature, and the QuicSession event path, with a new test file; it touches no auth or injection surface but does touch native callback lifecycles and a vendored protocol library. A confirmed test-cleanup nit is posted inline, and earlier optional concerns about mapping LSQ_DG_UNSENT to 'lost' remain a design choice the author acknowledged in the description, so a human look at the lsquic semantics is still worthwhile rather than approval.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
|
Updated 1:46 AM PT - Oct 18th, 2026
❌ @robobun, your commit 0840e16 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44680That installs a local version of the PR into your bun-44680 --bun |
…ests when a step fails The relay harness and the 0-RTT test now keep what they open in an `AsyncDisposableStack`. A handshake that fails releases the listening endpoint, the relay sockets and the sessions, and the test fails with the cause.
…-datagram-status-by-id
`cd test && bun check` reports no type error on main. The new file passes it too: `server.address!`, a `SocketAddress` for the relay, and `as SessionOptions` for `verifyPeer`, as in the other files of this directory.
Problem
ondatagramstatus(id, status)names the wrong id when a datagram is lost ahead of others. The network loses 1 of 6: bun reports1 acknowledgedand2 lost.(count, acked)for a packet, andQuicSessionpops that many ids from a send-order queue (inflight_datagrams,src/runtime/node/quic/session.rs:1298).Fix
on_dg_writenames the frame with the newlsquic_conn_set_datagram_id(). lsquic keeps the id in the frame record and reports(id, status)when the packet is acknowledged, lost, or dropped before the send ('lost').dgram_iddoes in ngtcp2 under Node.QuicSessionkeeps no id queue.test/js/node/quic/quic-datagram.test.ts(8 of 10 fail on main), the 235 vendoredtest-quic-*tests, the HTTP/3 suites. Self-reviewed: 21 concerns raised, 19 addressed. Not taken: adestroyedcheck for two other loops (not reproduced, separate change).Background
Downsides
'lost'. Node keeps it queued. An app that resends on'lost'got about 1000 callbacks in a 3 s outage (86 on main).Notes
Other changes a user can see
stats.datagramsSentcounts a datagram when lsquic frames it. On main the count came one event-loop pass later.datagramsSentcounts ends indatagramsAcknowledgedordatagramsLoston a session that continues. On main a datagram that lsquic dropped before the send was counted in neither.ondatagramstatusthat destroys the session (a throw does) ends the statuses of that pass. On main each further datagram of the same packet raised an uncaughtERR_INTERNAL_ASSERTION("Unexpected datagram status event").quic.session.receive.datagram.statusgets the same corrected pairs.The lsquic change
patches/lsquic/node-quic-accessors.patchis a patch file, so its diff here is a diff of a diff. This is what it changes in lsquic, condensed:LSQ_DG_UNSENTsites are the places where lsquic discards a DATAGRAM frame that it framed and did not send. After a retransmission timeout lsquic sends at most two packets until an ACK arrives. It drops a waiting packet that holds only datagrams (lsquic_send_ctl_squeeze_sched). It cuts the DATAGRAM frame out of a waiting packet that it gives a new number (PO_REPACKNO).lsquic_send_ctl_drop_scheduled, or at connection cleanup. Node reports nothing there.coalesce-batch-drop,connection-close-pns,goaway-accept-uni-streams) apply on top of it unchanged.Designs that were weighed
datagramsSentstays 13 against 3 acknowledged and 1 lost.session.rs(the idea in node:quic on lsquic — Node v26 compat, HTTP/3 #32602). It is a second book beside the packet, and a packet that lsquic drops or renumbers leaves it stale.send_ctl_destroy_packet. It adds a bit test for each destroyed packet on the ACK path of every connection, and it fires at close.Measurements
Release builds of the merge base 13a98b0 and of this branch, linux x64.
perf,valgrind,straceandbloatyare not installed on this machine, so the instruction counts are static counts fromobjdump.struct ietf_full_conn, release: 10232 -> 10232 bytes (thecallocimmediate inlsquic_ietf_full_conn_client_newand_server_new). Debug: 10280 -> 10280.Bun.serveHTTP/3 andfetchuse this struct too.lsquic_send_ctl_sent_packet134 -> 134,squeeze_sched129 -> 224,next_packet_to_send266 -> 362,got_ack963 -> 1001,write_datagram99 -> 99. Each added instruction is behind theQUIC_FTBIT_DATAGRAMtest.size).lsquic_send_ctl_got_ack4123 -> 4367 bytes,write_datagram405 -> 418 bytes.QuicSession880 -> 848 bytes.SessionEvent136 -> 136 bytes.sentevents and 1013 status events -> 0 and 516.Bun.serveHTTP/3 run of 2,247 sent packets: 0.datagramsSent/Acknowledged/Lost13/3/1 -> 13/3/10, in 3 of 3 runs.'lost', in a 3 s outage. With one chain of resends: 2 -> 11 to 13'lost'callbacks. With its own datagram each 10 ms too: 85 or 86 -> 805 to 1040 (5 runs). The count depends on timing: an earlier build of this branch gave 466 to 500. The packets on the wire are the same (88 to 90 datagram packets).Tests
test/js/node/quic/quic-datagram.test.tsputs a UDP relay between a client and a server in one process. The relay loses or keeps packets that it selects by length. This is the loss-injection harness that #32602 said the fix needs.ERR_INTERNAL_ASSERTION. No test fails by a timeout.node:quic. The later commits change only the test file: how the tests fail, how they release what they open, and their types. After them: debug 12 of 12, and 10 of 10 with the load. The release runs are from before them.test/(bun check js/node/quic/quic-datagram.test.tsintest/: no type errors).test/js/node/test/parallel/test-quic-*.mjs(same result on both release builds: 233 pass,test-quic-stream-idle-timeout.mjsandtest-quic-h3-stream-idle-timeout.mjsfail on both),test/js/node/quic/(30 pass, debug),serve-http3.test.ts(72 pass, 1 skip),fetch-http3-client.test.ts(65),fetch-http3-adversarial.test.ts(27),fetch-http3-cold-post.test.ts(1 pass, 1 skip).Cases that stay as they are
'lost'when its ACK is late. Same on main.'abandoned'changes no counter. Node counts it indatagramsLost.Follow-ups (not in this PR)
LSQ_DG_UNSENTand the resend cycle above go away, and bun matches Node.destroyedcheck of their own. A stray callback from the ticket loop did not reproduce: the second ticket of a connection is delivered 500 ms after the first, in a pass that has the check. These are the two self-review concerns that this PR does not take.maxDatagramSendAttemptsis validated inquic.tsand read nowhere.Open PRs that touch the same lines
on_dg_writeuser. It pays nothing if it does not set an id.